Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #5135: adding an edit method to each model object #5138

Merged
merged 3 commits into from
Sep 18, 2023

Conversation

shawkins
Copy link
Contributor

@shawkins shawkins commented May 16, 2023

Description

In scope of #5135

This will enable users / us to do something like pod.edit().build() as a clone operation.

I've also opened sundrio/sundrio#376 so that we can align to the common BaseFluent.builderOf method for obtaining a builder. Or in the interim we may need to keep our similar logic and just update it some more and make it static. From there we'll use that in ResourceHandlerImpl and in Serialization (for cloning).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@shawkins
Copy link
Contributor Author

Took another pass at this and it won't quite line up with what's in sundrio. The initial commit is referencing io.sundr.builder.Editable, and generates / builds just fine. However our generated sundrio logic will actually reference io.fabric8.kubernetes.api.builder.Editable. If I try to reference io.fabric8.kubernetes.api.builder.Editable in the jsonschema2pojo that results in an NPE.

One option is to leave this decoupled from sundrio and not implement the interface. Users would still do:

pod.toBuilder().... rather than new PodBuilder(pod)...

but the dependent builder logic would not find these methods and would still use reflection if needed.

Another option is to somehow move the io.fabric8.kubernetes.api.builder classes into kubernetes-model-common.

@shawkins shawkins force-pushed the iss5135 branch 2 times, most recently from f26f286 to 5c6e460 Compare June 20, 2023 13:40
@shawkins
Copy link
Contributor Author

Another option is to somehow move the io.fabric8.kubernetes.api.builder classes into kubernetes-model-common.

Updated the pr with what that would look like. A dummy class is used to trigger the builder package inclusion in the common module - the class and generated output are removed from the jar.

What's not shown is that the core model needs regenerated to flip the generateBuilderPackage value to false.

In total we're marking the class as Editable and adding an edit method with an alias of toBuilder. Obviously the toBuilder method could be handled upstream as a default on the interface.

@manusa @iocanel @rohanKanojia @metacosm does this messiness seem worth it?

@shawkins
Copy link
Contributor Author

I'll see if I can resolve then NPE this upstream in sundrio before moving forward with this hack based approach.

@shawkins
Copy link
Contributor Author

shawkins commented Jul 5, 2023

I'll see if I can resolve then NPE this upstream in sundrio before moving forward with this hack based approach.

Opened sundrio/sundrio#396 but could not see how to immediately resolve it. We'll probably need to use this workaround if we want to make the pojos editable any time soon.

@iocanel
Copy link
Member

iocanel commented Jul 12, 2023

Some thoughts:

On BaseFluent.builderOf

AFAIR this uses Class.forName and its something I would really like to avoid. Ideally, this should move down one level into the concrete fluent (where it can be re-implemented without reflection).

kubernetes model builder package

One way of solving the chicken and egg thing with jsonschema2pojo would be to generate the package in separate jar, that jsonschema2pojo can depend on. The rest of the model can depend on that jar and not regenerate the model.

A completely different approach would be to add the method using a lombok style byte code manipulation (however this would have to be done on sundrio).

edit() vs toBuilder()

It doesn't really matter which one will be used, but ...
It would be really nice if whatever method is used is expressed using an interface. It would make things easier when parsing files, and apply visitors etc.

@shawkins
Copy link
Contributor Author

AFAIR this uses Class.forName and its something I would really like to avoid. Ideally, this should move down one level into the concrete fluent (where it can be re-implemented without reflection).

The first check in builderOf is for if the item is Editable.

One way of solving the chicken and egg thing with jsonschema2pojo would be to generate the package in separate jar, that jsonschema2pojo can depend on. The rest of the model can depend on that jar and not regenerate the model.

That is in this pr - a dummy class is used to trigger the creation of the fabric8 builder package in a different class.

It would be really nice if whatever method is used is expressed using an interface. It would make things easier when parsing files, and apply visitors etc.

If you are open to it upstream, then toBuilder could be added as a default method.

From a end user perspective it's still a little awkward as an interface (we talked about this a little on a sundrio issue) - as there's currently no additional bounds, you'll need to at least cast to VisitableBuilder.

@shawkins shawkins marked this pull request as ready for review September 11, 2023 12:04
@shawkins
Copy link
Contributor Author

@metacosm @manusa marking this as ready. A follow-up will be to add the toBuilder method to the Editable interface.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me.

@manusa manusa added this to the 6.9.0 milestone Sep 18, 2023
@manusa manusa merged commit a3efaa3 into fabric8io:main Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants